-
Notifications
You must be signed in to change notification settings - Fork 46.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Transition Tracing] Refactor Transition Tracing Root Code #24766
Conversation
Comparing: 88574c1...527baa8 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
} | ||
|
||
transitions.forEach(transition => { | ||
incompleteTransitions.set(transition, new Map()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set the Map to null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move some of this code to prepareFreshStack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of pushing a Map() push a marker instance
pendingSuspenseBoundaries: PendingSuspenseBoundaries | null, | ||
): void { | ||
if (enableTransitionTracing) { | ||
const markerInstance = {transitions, pendingSuspenseBoundaries}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't create just pass the actual instance
@@ -909,7 +910,10 @@ function updateTracingMarkerComponent( | |||
} | |||
} | |||
|
|||
pushTracingMarker(workInProgress); | |||
const instance = workInProgress.stateNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to always cast to a specific type when reading from .stateNode
transitions.forEach(transition => { | ||
// We need to create a new map here because we only have access to the | ||
// object instance in the commit phase | ||
root.incompleteTransitions.set(transition, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if this transition already exists in the map? Wouldn't this override it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it shouldn't because we only push the root once and we only push the transition when it has just been added but I'll add a check here just to make sure!
2e6563f
to
ff6fa5e
Compare
@@ -3702,7 +3714,10 @@ function attemptEarlyBailoutIfNoScheduledUpdate( | |||
} | |||
case TracingMarkerComponent: { | |||
if (enableTransitionTracing) { | |||
pushTracingMarker(workInProgress); | |||
const instance: TracingMarkerInstance = workInProgress.stateNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const instance: TracingMarkerInstance = workInProgress.stateNode; | |
const instance: TracingMarkerInstance | null = workInProgress.stateNode; |
6975b0b
to
a320d29
Compare
28b39c5
to
2c6641b
Compare
…acebook#24766)" This reverts commit 4012963. This PR refactors the transition tracing root code by reusing the tracing marker code. Namely it: * Refactors the tracing marker code so that it takes a tracing marker instance instead of a tracing marker fiber and rename the stack to `markerInstance` instead of `tracingMarker` * Pushes the root code onto the stack * Moves the instantiation of `root.incompleteTransitions` to the begin phase when we are pushing the root to the stack rather than in the commit phase
…acebook#24766)" This reverts commit 4012963 because it's failing on main, likely due to conflict with something that landed before the PR was merged. Need to rebase and fix.
Summary: This sync includes the following changes: - **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>// - **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>// - **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>// - **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>// - **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>// - **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>// - **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>// - **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>// - **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>// - **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>// - **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>// - **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>// - **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>// - **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>// - **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>// - **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>// - **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>// - **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>// - **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>// - **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>// Changelog: [General][Changed] - React Native sync for revisions 229c86a...c1f5884 Reviewed By: mdvacca, GijsWeterings Differential Revision: D38904311 fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
Summary: This sync includes the following changes: - **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>// - **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>// - **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>// - **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>// - **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>// - **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>// - **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>// - **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>// - **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>// - **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>// - **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>// - **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>// - **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>// - **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>// - **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>// - **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>// - **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>// - **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>// - **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>// - **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>// Changelog: [General][Changed] - React Native sync for revisions 229c86a...c1f5884 Reviewed By: mdvacca, GijsWeterings Differential Revision: D38904311 fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
Summary: This sync includes the following changes: - **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([facebook#24846](facebook/react#24846)) //<Andrew Clark>// - **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([facebook#24830](facebook/react#24830)) //<Luna Ruan>// - **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766))" ([facebook#24829](facebook/react#24829)) //<Andrew Clark>// - **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766)) //<Luna Ruan>// - **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([facebook#24585](facebook/react#24585)) //<Andrew Clark>// - **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([facebook#24749](facebook/react#24749)) //<Andrew Clark>// - **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([facebook#24817](facebook/react#24817)) //<Andrew Clark>// - **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([facebook#24806](facebook/react#24806)) //<Luna Ruan>// - **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([facebook#24801](facebook/react#24801)) //<Luna Ruan>// - **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([facebook#24686](facebook/react#24686)) //<Luna Ruan>// - **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([facebook#24784](facebook/react#24784)) //<Josh Story>// - **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([facebook#24776](facebook/react#24776)) //<Luna Ruan>// - **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([facebook#24765](facebook/react#24765)) //<Luna Ruan>// - **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([facebook#24754](facebook/react#24754)) //<Sebastian Markbåge>// - **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([facebook#24752](facebook/react#24752)) //<Sebastian Markbåge>// - **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([facebook#24753](facebook/react#24753)) //<Sebastian Markbåge>// - **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([facebook#24751](facebook/react#24751)) //<Sebastian Markbåge>// - **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([facebook#24732](facebook/react#24732)) //<Luna Ruan>// - **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([facebook#24742](facebook/react#24742)) //<Mengdi Chen>// - **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([facebook#24612](facebook/react#24612)) //<Kerim Büyükakyüz>// Changelog: [General][Changed] - React Native sync for revisions 229c86a...c1f5884 Reviewed By: mdvacca, GijsWeterings Differential Revision: D38904311 fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
This PR refactors the transition tracing root code by reusing the tracing marker code. Namely it:
markerInstance
instead oftracingMarker
root.incompleteTransitions
to the begin phase when we are pushing the root to the stack rather than in the commit phaseDepends on #24686 only the
refactor root
commit should be looked at here